Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kotlin-Spring template fixes/improvements #3007

Merged
merged 4 commits into from
Jun 3, 2019
Merged

Conversation

drejc
Copy link
Contributor

@drejc drejc commented May 27, 2019

Description of the PR

Fixed consumes/produces comma placement.
Sometimes following happened: produces = ["application/json", ],

  • a comma was placed after last element. I susupect that an empty consumes/produces element is somehow added to the list.

Changed default response code in interface returnValue from HttpStatus.OK to first response code in definition. Allowing other responses, like created, accepted ...

drejc added 3 commits May 27, 2019 10:04
…n definition.

Allowing also other responses 201, 202 ...
…n definition.

Allowing also other responses 201, 202 ...
@wing328
Copy link
Member

wing328 commented May 27, 2019

@drejc thanks for the PR. Can you please run ./bin/kotlin-springboot-petstore-server.sh to update the Kotlin samples so that the CI can verify the result?

@wing328
Copy link
Member

wing328 commented May 27, 2019

cc @jimschubert (2017/09), @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)

@wing328 wing328 added this to the 4.0.1 milestone May 27, 2019
@drejc
Copy link
Contributor Author

drejc commented May 27, 2019

I have run the ./bin/kotlin-springboot-petstore-server.sh and the newly generated files are "broken" due to missing success response definitions in /2_0/petstore.yaml file.

Where for instance there is the following definition:

delete:
      tags:
        - user
      summary: Delete user
      description: This can only be done by the logged in user.
      operationId: deleteUser
      produces:
        - application/xml
        - application/json
      parameters:
        - name: username
          in: path
          description: The name that needs to be deleted
          required: true
          type: string
      responses:
        '400':
          description: Invalid username supplied
        '404':
          description: User not found

The generated code is:

@ApiOperation(
           value = "Deletes a pet",
           nickname = "deletePet",
           notes = "",
           authorizations = [Authorization(value = "petstore_auth", scopes = [AuthorizationScope(scope = "write:pets", description = "modify pets in your account"), AuthorizationScope(scope = "read:pets", description = "read your pets")])])
   @ApiResponses(
           value = [ApiResponse(code = 400, message = "Invalid pet value")])
   @RequestMapping(
           value = ["/pet/{petId}"],
           method = [RequestMethod.DELETE])
   fun deletePet(@ApiParam(value = "Pet id to delete", required=true) @PathVariable("petId") petId: Long
,@ApiParam(value = "" ) @RequestHeader(value="api_key", required=false) apiKey: String?
): ResponseEntity<Unit> {
       return ResponseEntity(service.deletePet(petId, apiKey), HttpStatus.valueOf(400))
   }

Previously it was: HttpStatus.OK

Now I didn't dive into swagger definition details if 200 is by default the expected default response, nevertheless either:

  • the definition must be updated
  • or a default response 200 as the first response added (if no 2** code present) to model prior to generating

@wing328
Copy link
Member

wing328 commented May 27, 2019

Now I didn't dive into swagger definition details if 200 is by default the expected default response, nevertheless either:

I've seen many spec without 200 response defined in the endpoints so I think we need a better way to handle this.

@drejc
Copy link
Contributor Author

drejc commented May 27, 2019

Yes I agree ... and I have seen a similar approach in /JavaSpring/api.mustache

where response codes are generated:

@ApiResponses(value = { {{#responses}}
        @ApiResponse(code = {{{code}}}, message = "{{{message}}}"{{#baseType}}, response = {{{baseType}}}.class{{/baseType}}{{#containerType}}, responseContainer = "{{{containerType}}}"{{/containerType}}){{#hasMore}},{{/hasMore}}{{/responses}} })
    {{#implicitHeaders}}

And the generated code:

@ApiOperation(value = "Delete purchase order by ID", nickname = "deleteOrder", notes = "For valid response try integer IDs with value < 1000. Anything above 1000 or nonintegers will generate API errors", tags={ "store", })
    @ApiResponses(value = { 
        @ApiResponse(code = 400, message = "Invalid ID supplied"),
        @ApiResponse(code = 404, message = "Order not found") })
    @RequestMapping(value = "/store/order/{order_id}",
        method = RequestMethod.DELETE)
    default ResponseEntity<Void> deleteOrder(@ApiParam(value = "ID of the order that needs to be deleted",required=true) @PathVariable("order_id") String orderId) {
        return new ResponseEntity<>(HttpStatus.NOT_IMPLEMENTED);
    }

I would suggest to manually add the 200 response as the first default response in case there is no other non-error response definition, as this is the expected behaviour.

This allows for customisation of 2** responses, and will not break existing APIs

Copy link
Contributor

@karismann karismann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm agree that petstore spec need to be improved to cover at least one successful status code for each operation. I will try to fill a PR for this purpose.

@karismann
Copy link
Contributor

FIY https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#responsesObject

The documentation is not necessarily expected to cover all possible HTTP response codes because they may not be known in advance. However, documentation is expected to cover a successful operation response and any known errors.

@wing328
Copy link
Member

wing328 commented May 28, 2019

I'm agree that petstore spec need to be improved to cover at least one successful status code for each operation. I will try to fill a PR for this purpose.

@karismann please ping me via Gitter to have a chat on this before filing a PR.

@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019
@wing328 wing328 merged commit caac76c into OpenAPITools:master Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants